Skip to content

Minor code readability improvements discovered on code review. - #1003

Closed
sgpinkus wants to merge 3 commits into
actions:mainfrom
sgpinkus:main
Closed

Minor code readability improvements discovered on code review.#1003
sgpinkus wants to merge 3 commits into
actions:mainfrom
sgpinkus:main

Conversation

@sgpinkus

@sgpinkus sgpinkus commented Jan 8, 2025 •
edited
Loading

Copy link
Copy Markdown

Description:
Minor code readability improvements discovered on code review.

  • Rename (Pipenv|Poetry)Cache "patterns" field to "cacheDependencyPath" to be consistent with PipCache, base class, and cache-factory.ts
  • Rm CacheDistributor.cacheDependencyPath field and pass back from computeKeys instead, simplifying CacheDistributor.restoreCache error check. Rm now not needed constant.ts.

Related issue:

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@sgpinkus
sgpinkus requested a review from a team as a code owner January 8, 2025 06:14
@sgpinkus
Readability improvement: rename PipenvCache,PoetryCache "patterns" fi…
1f3a570 
…eld to "cacheDependencyPath" to be consistent with base class and cache-factory.
@sgpinkus
Readability improvement: Move CacheDistributor constructor params to …
0024ce0 
…readonly abstract fields of concrete cache implementations. Remove empty CacheDistributor constructor.
@sgpinkus
Code improvemnt: Rm CacheDistributor.cacheDependencyPath field and pa…
d99639b 
…ss back from computeKeys instead, simplifying CacheDistributor.restoreCache error check. Rm now uneeded constant.ts file.
@v-priyagupta108

v-priyagupta108 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Hi @sgpinkus, thank you for taking the time to review the codebase and submit this PR. We really appreciate the effort!

After reviewing the changes, we've decided not to move forward with this PR. The main concern is that the computeKeys() refactor changes the pip error message behavior. When neither the primary nor backup dependency path matches, users would only see **/pyproject.toml instead of the current **/requirements.txt or **/pyproject.toml. This makes it harder for users to debug cache configuration issues.

While the naming consistency improvements (renaming patterns → cacheDependencyPath) are a nice touch, we don't feel the readability gains are significant enough to warrant the change at this time.

Thank you for your effort here, and please feel free to contribute again anytime!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sgpinkus @v-priyagupta108